-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make no-mocha-arrows rule fixable #112
Conversation
Travis CI workers don't start... Could you restart them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rhysd! Thanks a lot for this :D
Could you add some additional tests for
- Generator functions (
*() => {}
if I recall correctly) - Async functions (
async () => {}
) - Functions with callbacks (
(done) => {}
)
}, | ||
{ | ||
code: 'it(() => assert(something, false))', | ||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [ { message: expectedErrorMessage, column: 1, line: 1 } ] | ||
errors: [ { message: expectedErrorMessage, column: 1, line: 1 } ], | ||
output: 'it(function () { assert(something, false); })' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output should be
it(function () { return assert(something, false); })
Imagine the user wrote a test for function that returns a Promise, and they just want to check that it resolves.
it(() => lib.resolves());
If it's changed to
it(() => {
lib.resolves();
});
then the test will passed even if the Promise rejects. It's much safer to change it to
it(function() {
return lib.resolves();
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you're correct. I'll fix this.
My TODO:
|
I fixed all the points. Could you review additional patches? |
// When 'async' specified, take care about the keyword. | ||
functionKeyword = 'async ' + functionKeyword; | ||
// Strip 'async (...)' to '(...)' | ||
paramsFullText = paramsFullText.slice(6); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both async () => {}
and async() => {}
are actually valid. This would create broken code if you find the latter form. (add a test for that obviously :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... It looks nice point. I need to improve code transform on async function.
}, | ||
{ | ||
code: 'it(async function () { assert(something, false) })', | ||
parserOptions: { ecmaVersion: 2017 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure you can do
ruleTester = new RuleTester({
parserOptions: {ecmaVersion: 2017}
});
and remove all the parserOptions override in individual test cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's much better. I don't know eslint toolchain very much. Thanks.
], | ||
|
||
invalid: [ | ||
{ | ||
code: 'it(() => { assert(something, false); })', | ||
parserOptions: { ecmaVersion: 6 }, | ||
errors: [ { message: expectedErrorMessage, column: 1, line: 1 } ] | ||
errors: [ { message: expectedErrorMessage, column: 1, line: 1 } ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually extract the common errors into a variable and just put it everywhere. Removes a lot of duplicated code. but @lo1tuma may not have the same preference.
var errors = [ { message: 'Do not pass arrow functions to it()', column: 1, line: 1 } ];
ruleTester.run('no-mocha-arrows', rules['no-mocha-arrows'], {
// ...
{
code: '...',
errors: errors
}
});
context.report({ | ||
node: node, | ||
message: 'Do not pass arrow functions to ' + name + '()', | ||
fix: function (fixer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function is starting to get pretty big, declare it somewhere else and pass the needed info
function fix(context, fnArg, ...) {
return function (fixer) {
var sourceCode = context.getSourceCode(),
paramsLeftParen = sourceCode.getFirstToken(fnArg),
// ...
};
}
// ...
context.report({
node: node,
message: // ...
fix: fix(context, fnArg, ...)
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree :)
Thank you @jfmengels for nice points. I'll fix them. |
I added extra 4 commits. Each commit corresponds to your review comment. Could you review them? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from this last almost esthetic comment, LGTM :)
Thanks for working on this, and sorry for the late review.
It's up to @lo1tuma to merge afterwards :)
|
||
}); | ||
|
||
es2017RuleTester.run('no-mocha-arrows', rules['no-mocha-arrows'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would have been simpler (and less confusing) to have only one ruleTester.run
call with the es6 settings, and just override the parser settings where needed (in the following cases).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel it's enough to use test runner using es2017 parser for every test. I'll make one test runner for tests.
@jfmengels I fixed the point 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
LGTM as well 👍 . Thanks everyone. |
Summary
I make
no-mocha-arrows
rule fixable with--fix
option ofeslint
command.Example
EDIT: Translation result of single expression body